Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: implement :read typed command #3966

Closed
wants to merge 5 commits into from
Closed

feat: implement :read typed command #3966

wants to merge 5 commits into from

Conversation

idursun
Copy link
Contributor

@idursun idursun commented Sep 25, 2022

This is a naive attempt at partially implementing #3796

Adds a typed command :read with alias :r (following vim) that reads the contents of a file into the current buffer.

At this stage, I just would like to validate my approach and obviously would love to finish it off after implementing some integration tests.

I also appreciate any pointers and guidance around reading the file contents into the buffer more efficiently. I am loading the file into a String for now.

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Sep 26, 2022
@pinpox
Copy link
Contributor

pinpox commented Oct 4, 2022

Thanks for working on this! Would it be possible to integrate this with the file picker? Might be a separate PR though

@idursun idursun force-pushed the master branch 2 times, most recently from 55a4b8e to 109fd55 Compare October 4, 2022 14:47
@idursun
Copy link
Contributor Author

idursun commented Oct 4, 2022

@kirawi I have copied and used a trimmed down version of helix_view::document::from_reader that is using encoding_rs::Decoder::decode_to_string. I have also added an integration test that is only testing the happy path.

I appreciate it if you can have a look and let me know if you suggest any changes.

@idursun idursun force-pushed the master branch 2 times, most recently from 504b26f to 43c1951 Compare October 4, 2022 16:26
@idursun idursun marked this pull request as ready for review October 4, 2022 21:54
the-mikedavis
the-mikedavis reviewed Oct 4, 2022
View reviewed changes
helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
helix-term/src/commands/typed.rs
}

/// Stripped down version of [`helix_view::document::from_reader`] which is adapted to use encoding_rs::Decoder::read_to_string
fn from_reader<R: std::io::Read + ?Sized>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates a lot of code between this function and document::from_reader. Can you factor out the shared parts and share the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I was following @kirawi's advice on this: #3966 (comment)

I can try working something out at the expense of dragging this PR further, or that work can be the subject of another PR. I am easy either way. It's all up to the maintainers at this point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can use read_to_string from document now:

helix/helix-view/src/document.rs

Lines 474 to 477 in 636c91c

pub fn read_to_string<R: std::io::Read + ?Sized>(
reader: &mut R,
encoding: Option<&'static Encoding>,
) -> Result<(String, &'static Encoding, bool), Error> {

helix-term/tests/test/commands.rs Outdated Show resolved Hide resolved
@sudormrfbin
Copy link
Member

sudormrfbin commented Oct 5, 2022 •
edited
Loading

How about :read-file-into-buffer? :read isn't very descriptive of what it reads, what it does to the read file, etc. We can still have the aliases for :read and :r.

@equals03 equals03 mentioned this pull request Oct 10, 2022
Closed
@the-mikedavis the-mikedavis linked an issue Oct 15, 2022 that may be closed by this pull request
Add command to read file contents into current buffer (vim's :r) #3796
Closed
@the-mikedavis the-mikedavis mentioned this pull request Oct 15, 2022
Closed
@idursun
Copy link
Contributor Author

idursun commented Oct 19, 2022

How about :read-file-into-buffer? :read isn't very descriptive of what it reads, what it does to the read file, etc. We can still have the aliases for :read and :r.

@sudormrfbin I have named it :read just to be aligned with vim's naming of the command.

@the-mikedavis
Copy link
Member

Vi :read is more general - you can read the output of shell commands like :read !echo "foo"

We already have shell_insert_output for that case so I think we should limit this command to reading from files and have a name that reflects that

@the-mikedavis the-mikedavis mentioned this pull request Oct 29, 2022
Closed
@kirawi kirawi mentioned this pull request Dec 3, 2022
Closed
@the-mikedavis the-mikedavis mentioned this pull request Jun 23, 2023
Merged
@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Jun 26, 2023
@the-mikedavis the-mikedavis removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 10, 2024
@the-mikedavis the-mikedavis added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Feb 10, 2024
@shaleh shaleh mentioned this pull request Apr 15, 2024
Merged
shaleh added a commit to shaleh/helix that referenced this pull request Apr 15, 2024
@shaleh
Implement read command.
c265117 
Resurrects helix-editor#3966. This is 100% @idursun's work. Using read_to_string from helix-editor#7431.
Co-authored-by: [email protected]
shaleh added a commit to shaleh/helix that referenced this pull request Apr 16, 2024
@shaleh
Implement read command.
f0f25b6 
Resurrects helix-editor#3966. This is 100% @idursun's work. Using read_to_string from helix-editor#7431.
Co-authored-by: [email protected]
@pascalkuthe
Copy link
Member

closing in favor of #10447

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kirawi kirawi kirawi left review comments

@the-mikedavis the-mikedavis the-mikedavis left review comments

@archseer archseer archseer left review comments

Assignees
No one assigned
Labels
A-helix-term Area: Helix term improvements S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add command to read file contents into current buffer (vim's :r)
7 participants
@idursun @pinpox @sudormrfbin @the-mikedavis @pascalkuthe @archseer @kirawi